Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop time when used as a tag or field key #7132

Merged
merged 1 commit into from
Aug 10, 2016

Conversation

jsternberg
Copy link
Contributor

The "time" field and tags are unqueryable so we prevent those from being
written so we don't have unreadable data.

Fixes #1834.

@mention-bot
Copy link

@jsternberg, thanks for your PR! By analyzing the annotation information on this pull request, we identified @e-dard to be a potential reviewer

@jsternberg jsternberg added this to the 1.1.0 milestone Aug 9, 2016
@gunnaraasen
Copy link
Member

Is this a breaking change? Even though a time tag/field cannot be queried, there may be clients expecting to be able to write a time tag/field and will break after this change. Seems like dropping the time tag/field and adding a warning in the logs or HTTP response would be a more compatible approach.

@jsternberg
Copy link
Contributor Author

That's a good point. I think this would classify as a breaking change.

Comments @pauldix is dropping it ok or should we find a way to make this queryable?

@@ -447,6 +447,17 @@ func (s *Shard) validateSeriesAndFields(points []models.Point) ([]*FieldCreate,

// get the shard mutex for locally defined fields
for _, p := range points {
// verify the tags and fields
tags := p.Tags()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't the right place to reject this as it will fail the whole batch instead of the point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would also have that issue with the max series error. We may want to look into that again if we have the time.

@pauldix
Copy link
Member

pauldix commented Aug 10, 2016

Dropping and adding in the log seems like an ok approach to keep anyone from breaking now. But longer term I think that makes it less usable for new users. If you fail the write then any new user coming in would get immediate feedback that what they're doing isn't supported. Users frequently don't have access to the logs.

I'm ok with the breaking change if we get it before 1.0. Otherwise, drop and warn would have to be it.

@jsternberg jsternberg force-pushed the js-1834-disallow-time-tag-and-field branch from 35fd188 to ff0bc30 Compare August 10, 2016 15:00
The "time" field and tags are unqueryable so we prevent those from being
written so we don't have unreadable data.
@jsternberg jsternberg force-pushed the js-1834-disallow-time-tag-and-field branch from ff0bc30 to 9621bee Compare August 10, 2016 15:02
@jsternberg jsternberg changed the title Disallow using time as a tag key or field key Drop time when used as a tag or field key Aug 10, 2016
@jsternberg
Copy link
Contributor Author

I dropped it and I added a TODO.md file to keep track of any issues we can't resolve completely until 2.0. This way, we can remember which things we would want to break when we get to that point and decide then if we want to do it or not.

@gunnaraasen
Copy link
Member

👍 the 2.0 TODO will be useful. Although it feels like it should be a GH issue of wiki page so updating it doesn't require a PR.

@jsternberg
Copy link
Contributor Author

@gunnaraasen my idea was that people would update it whenever we made a change like this where we worked around something. I'll merge this and we can always remove it later if we decide that a wiki would be better.

@jsternberg jsternberg merged commit c60fa73 into master Aug 10, 2016
@jsternberg jsternberg deleted the js-1834-disallow-time-tag-and-field branch August 10, 2016 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants